Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughClient configuration now uses one endpoint for REST and realtime traffic. Endpoint values determine primary and fallback hosts. Builders, tests, examples, and sandbox integrations use endpoint configuration. URL construction brackets IPv6 hosts. ChangesEndpoint Routing
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ClientOptions
participant HttpCore
participant ConnectionManager
participant Hosts
participant Defaults
ClientOptions->>HttpCore: Provide endpoint options
HttpCore->>Hosts: Construct Hosts(options)
ClientOptions->>ConnectionManager: Provide endpoint options
ConnectionManager->>Hosts: Construct Hosts(options)
Hosts->>Defaults: Resolve primary domain and fallback hosts
Defaults-->>Hosts: Return endpoint-derived hosts
Hosts-->>HttpCore: Return resolved host configuration
Hosts-->>ConnectionManager: Return resolved host configuration
Merge Risk: ⚪ Minimal · up to No merge-blocking issue is identified; the PR is ready for normal checks. Pre-merge checks |
|
There was a problem hiding this comment.
🟡 Changes recommended
Accepted IPv6 endpoints produce malformed transport URLs and cannot connect.
1 open finding
What changed in this PR
This PR replaces separate host and environment settings with one endpoint for REST and realtime clients, aligning the SDK with the new host-routing model.
Changes:
- Resolves primary and fallback hosts from
endpoint. - Migrates builders, tests, examples, and integration helpers to the new option.
- Adds upgrade and test-setup guidance.
| File | Description |
|---|---|
uts/src/test/kotlin/io/ably/pubsub/uts/integration/standard/IntegrationInfraSmokeTest.kt |
Uses the sandbox endpoint. |
uts/src/main/kotlin/io/ably/pubsub/uts/infra/integration/SandboxApp.kt |
Defines the sandbox endpoint. |
uts/src/main/kotlin/io/ably/pubsub/uts/infra/integration/proxy/ProxySession.kt |
Routes proxy clients through endpoint. |
uts/README.md |
Updates integration setup guidance. |
UPGRADING.md |
Documents endpoint migration. |
server/src/test/java/io/ably/pubsub/server/PubSubServerTest.java |
Migrates server builder test. |
server/src/main/java/io/ably/pubsub/server/PubSubServer.java |
Replaces legacy builder methods. |
pubsub-adapter/src/test/kotlin/com/ably/Utils.kt |
Migrates local test client. |
liveobjects/src/test/kotlin/io/ably/pubsub/liveobjects/uts/proxy/ObjectsFaultsTest.kt |
Migrates direct sandbox client. |
liveobjects/src/test/kotlin/io/ably/pubsub/liveobjects/uts/integration/ObjectsSyncTest.kt |
Migrates sync test client. |
liveobjects/src/test/kotlin/io/ably/pubsub/liveobjects/uts/integration/ObjectsLifecycleTest.kt |
Migrates lifecycle test client. |
liveobjects/src/test/kotlin/io/ably/pubsub/liveobjects/uts/integration/Helpers.kt |
Migrates HTTP provisioning. |
liveobjects/src/test/kotlin/io/ably/pubsub/liveobjects/integration/setup/Sandbox.kt |
Migrates sandbox setup. |
lib/src/test/kotlin/io/ably/pubsub/uts/integration/standard/realtime/TokenRequestTest.kt |
Migrates token test clients. |
lib/src/test/kotlin/io/ably/pubsub/uts/integration/standard/realtime/ChannelHistoryTest.kt |
Migrates history test client. |
lib/src/test/java/io/ably/pubsub/transport/HostsTest.java |
Tests endpoint fallback behavior. |
lib/src/test/java/io/ably/pubsub/transport/DefaultsTest.java |
Tests endpoint resolution. |
lib/src/test/java/io/ably/pubsub/test/realtime/RealtimeJWTTest.java |
Migrates JWT test options. |
lib/src/test/java/io/ably/pubsub/test/realtime/RealtimeInitTest.java |
Migrates initialization test. |
lib/src/test/java/io/ably/pubsub/test/realtime/RealtimeHttpHeaderTest.java |
Migrates WebSocket test options. |
lib/src/test/java/io/ably/pubsub/test/realtime/RealtimeConnectFailTest.java |
Migrates failure test endpoints. |
lib/src/test/java/io/ably/pubsub/test/realtime/ConnectionManagerTest.java |
Updates connection host assertions. |
lib/src/test/java/io/ably/pubsub/test/http/HttpTimeTest.java |
Migrates invalid-host test. |
lib/src/test/java/io/ably/pubsub/test/http/HttpTest.java |
Updates HTTP host tests. |
lib/src/test/java/io/ably/pubsub/test/http/HttpRequestTest.java |
Migrates request test endpoints. |
lib/src/test/java/io/ably/pubsub/test/http/HttpProxyTest.java |
Disables fallback for a proxy test. |
lib/src/test/java/io/ably/pubsub/test/http/HttpJWTTest.java |
Derives JWT test environment. |
lib/src/test/java/io/ably/pubsub/test/http/HttpInitTest.java |
Tests new HTTP host resolution. |
lib/src/test/java/io/ably/pubsub/test/http/HttpHeaderTest.java |
Migrates local HTTP tests. |
lib/src/test/java/io/ably/pubsub/test/http/HttpErrorTest.java |
Migrates local error tests. |
lib/src/test/java/io/ably/pubsub/test/http/HttpClientTest.java |
Migrates fallback test endpoints. |
lib/src/test/java/io/ably/pubsub/test/http/HttpAuthTest.java |
Migrates authentication test options. |
lib/src/test/java/io/ably/pubsub/test/common/Setup.java |
Sets test endpoints centrally. |
lib/src/main/java/io/ably/pubsub/types/ClientOptions.java |
Adds endpoint; removes legacy options. |
lib/src/main/java/io/ably/pubsub/transport/Hosts.java |
Derives hosts from client options. |
lib/src/main/java/io/ably/pubsub/transport/Defaults.java |
Resolves endpoint domains and fallbacks. |
lib/src/main/java/io/ably/pubsub/transport/ConnectionManager.java |
Uses shared host resolution. |
lib/src/main/java/io/ably/pubsub/http/HttpCore.java |
Uses shared host resolution. |
lib/src/main/java/io/ably/pubsub/debug/DebugOptions.java |
Copies the endpoint option. |
examples/src/main/kotlin/com/ably/example/screen/MainScreen.kt |
Detects the sandbox endpoint. |
examples/src/main/kotlin/com/ably/example/MainActivity.kt |
Configures the sandbox endpoint. |
device/src/commonMain/kotlin/io/ably/pubsub/device/PubSubDevice.kt |
Replaces legacy builder methods. |
core-android/src/main/java/io/ably/pubsub/push/ActivationContext.java |
Updates an options TODO comment. |
core-android/src/androidTest/java/io/ably/pubsub/test/android/AndroidSuite.java |
Migrates Android HTTP test. |
CONTRIBUTING.md |
Updates test configuration guidance. |
.claude/skills/uts-to-kotlin/SKILL.md |
Updates test-translation guidance. |
.claude/skills/uts-to-kotlin/references/objects-mapping.md |
Updates sandbox mapping guidance. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (isHostname(endpoint)) { | ||
| return endpoint; |
There was a problem hiding this comment.
@coderabbitai please check how mandatory is this with respect to spec and rest of the code, check if current fix is enough, accordingly expand on the above
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline lib/src/main/java/io/ably/pubsub/http/HttpUtils.java --items all
ast-grep outline lib/src/main/java/io/ably/pubsub/transport/WebSocketTransport.java --items all
ast-grep outline lib/src/test/java/io/ably/pubsub/http/HttpUtilsTest.java --items all
rg -n -C3 'hostForUrl|buildURL\(|new URI\(|new URL\(|InetSocketAddress\(|getHost\(' lib/src/main/java --glob '*.java'
rg -n -C3 'REC1b2|ADR.?119|::1|IPv6' --glob '*.java' --glob '*.md' --glob '*gradle*'Length of output: 22332
🤖 web_search executed:
Ably SDK specification REC1b2 endpoint hostname contains "::" ADR-119 IPv6; RFC 3986 IPv6 host square brackets URI authority
Length of output: 796
🏁 Script executed:
#!/bin/bash
sed -n '185,220p' lib/src/main/java/io/ably/pubsub/http/HttpUtils.java
sed -n '110,160p' lib/src/main/java/io/ably/pubsub/transport/WebSocketTransport.java
ast-grep outline lib/src/main/java/io/ably/pubsub/http/HttpCore.java --items all --match 'httpExecute|retry|Retry'
rg -n -C4 'getHost\(|getAuthority\(|InetSocketAddress\(|URI\.create\(|new URI\(|new URL\(|wsUri|setHost\(' --glob '*.java' --glob '*.kt' -g '!**/test/**' -g '!**/androidTest/**'
fd -i 'WebSocket|HttpEngine|HttpClient|ProxySocket'
gh api repos/ably/ably-common/contents --jq '.[].name'Length of output: 26775
🏁 Script executed:
#!/bin/bash
ast-grep outline lib/src/main/java/io/ably/pubsub/http/HttpCore.java --items all
rg -n -C5 'buildURL|url\.|new HttpRequest|preferred|fallback|host =' lib/src/main/java/io/ably/pubsub/http/HttpCore.java
for file in network-client-okhttp/src/main/java/io/ably/pubsub/network/OkHttpEngine.java network-client-okhttp/src/main/java/io/ably/pubsub/network/OkHttpWebSocketEngine.java lib/src/test/java/io/ably/pubsub/transport/WebSocketTransportTest.java; do
ast-grep outline "$file" --items all
done
rg -n -C4 '\.url\(|create\(|IPv6|::1|ws://|wss://' network-client-okhttp/src/main/java/io/ably/pubsub/network/OkHttpEngine.java network-client-okhttp/src/main/java/io/ably/pubsub/network/OkHttpWebSocketEngine.java lib/src/test/java/io/ably/pubsub/transport/WebSocketTransportTest.java
gh api 'repos/ably/ably-common/git/trees/main?recursive=1' --jq '.tree[].path' | grep -Ei 'spec|feature|119|endpoint'Length of output: 11442
@sacOO7 IPv6 bracketing is required for valid transport URLs. The current fix is sufficient for the URL-formatting defect identified above. An actual IPv6 request and connection test would add confidence, but I would not make those tests mandatory for this fix.
Specification requirement
The endpoint contract supplied for this PR treats values containing :: as hostnames and preserves them unchanged. Therefore, Defaults.getPrimaryDomain("::1") returning "::1" is correct. Do not add brackets in Defaults.
When a transport places that address in a URL, RFC 3986, Section 3.2.2 requires brackets:
Raw host: ::1
REST URL: http://[::1]:8080/time
WebSocket URL: ws://[::1]:8080/
This is a URL syntax requirement, not an optional formatting choice. I could not independently retrieve the authoritative REC1/ADR-119 text, so the Ably-specific conclusion relies on the endpoint contract supplied here.
Current implementation
I verified these paths:
HttpUtils.hostForUrlbrackets colon-containing hosts and preserves already-bracketed hosts.HttpUtils.buildURLapplies the helper when constructing REST endpoint URLs.WebSocketTransport.connectapplies the same helper before passing the URL to the WebSocket engine.- The default WebSocket engine uses
URI.create(url). The OkHttp WebSocket engine passes the URL toRequest.Builder.url(url). - Host selection retains the original host.
WebSocketTransport.getHost()still returnsparams.host.
The fix therefore operates at the correct boundary. It covers HTTP/HTTPS and WS/WSS without changing host comparisons. No further change to Defaults.java is needed for this finding.
Test coverage
HttpUtilsTest covers unbracketed IPv6, already-bracketed IPv6, unchanged non-IPv6 hosts, and HTTP URL host/port parsing.
The most useful small addition would be a mocked WebSocketTransport test that asserts the URL passed to WebSocketEngine.create contains [::1], while getHost() remains "::1".
Real IPv6 loopback tests would check network-engine behavior beyond URL construction. They are useful additional coverage, but the evidence available does not establish them as a specification requirement or a blocker for this formatting fix.
This assessment is source-level verification. I did not run the tests or establish end-to-end IPv6 connectivity.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai so we are good with current changes right?
There was a problem hiding this comment.
@sacOO7 Yes—the current changes are sufficient for this IPv6 URL-formatting issue, based on the source review.
Keep Defaults.getPrimaryDomain("::1") unchanged. The REST and WebSocket URL builders now add the required brackets.
Additional WebSocket or IPv6 loopback tests are optional coverage, not blockers for this fix. This does not establish end-to-end IPv6 connectivity or approval of the entire PR.
You are interacting with an AI system.
2.0 adopts the REC1/REC2 host model (ADR-119): a single ClientOptions.endpoint selects where both REST requests and the realtime connection go, and the legacy environment, restHost and realtimeHost options are removed outright, so old code fails to compile rather than silently connecting to production. - endpoint resolves as: unset -> main.realtime.ably.net; a routing policy name -> [name].realtime.ably.net; nonprod:[name] -> [name].realtime.ably-nonprod.net; a hostname (contains '.' or '::', or is localhost) -> used as given. Default fallbacks follow the endpoint ([name].[a-e].fallback.ably-realtime[-nonprod].com) and a hostname has none. - Hosts now resolves from ClientOptions alone, so HttpCore and ConnectionManager share one primary domain. The restHost/environment conflict check, the rest.ably.io/realtime.ably.io defaults and the <env>-[a-e]-fallback hosts go with the legacy options. - An explicit fallbackHosts always replaces the defaults (REC2a2), and a custom port or tlsPort no longer suppresses the default fallbacks, as the spec has no such rule. - The server and device builders swap restHost(), realtimeHost() and environment() for endpoint(). The tests move to endpoint throughout: ABLY_ENV, ABLY_REST_HOST and ABLY_REALTIME_HOST become ABLY_ENDPOINT (default nonprod:sandbox), UTS and LiveObjects sandbox clients use the new SandboxApp.sandboxEndpoint, and mock/proxy clients use a localhost endpoint. Tests of the legacy host derivation and option conflicts are replaced by DefaultsTest/HostsTest cases for each endpoint form, custom fallbackHosts, and ports no longer disabling fallbacks. UPGRADING.md gains the migration table and the firewall allowlist note. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
3298eb3 to
f0f58a9
Compare

2.0 adopts the REC1/REC2 host model (ADR-119): a single
ClientOptions.endpoint selects where both REST requests and the realtime
connection go, and the legacy environment, restHost and realtimeHost
options are removed outright, so old code fails to compile rather than
silently connecting to production.
endpoint resolves as: unset -> main.realtime.ably.net; a routing policy
name -> [name].realtime.ably.net; nonprod:[name] ->
[name].realtime.ably-nonprod.net; a hostname (contains '.' or '::', or
is localhost) -> used as given. Default fallbacks follow the endpoint
([name].[a-e].fallback.ably-realtime[-nonprod].com) and a hostname has
none.
Hosts now resolves from ClientOptions alone, so HttpCore and
ConnectionManager share one primary domain. The restHost/environment
conflict check, the rest.ably.io/realtime.ably.io defaults and the
-[a-e]-fallback hosts go with the legacy options.
An explicit fallbackHosts always replaces the defaults (REC2a2), and a
custom port or tlsPort no longer suppresses the default fallbacks, as
the spec has no such rule.
The server and device builders swap restHost(), realtimeHost() and
environment() for endpoint().
Summary by CodeRabbit
New Features
Documentation
Compatibility